Clean up output, implement timeout, do not update the whole index when deploying a branch #127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 8 commits
April 9, 2019 15:45
Update doMirrorUpdate to be able to pass a branch name to it. Pass an empty branch when we want to deploy the whole thing, and pass only a specific branch when `-branch=...` has been specified. `git fetch` does not lock the whole index and only pulls down the branch which avoids the whole plethora of issues when more than one instance of g10k is running. For example, we could run into issues like this when `git remote update --prune` is running more than once: error: Ref refs/heads/test is at 1d793f181f3a460916ce1ab5dc33e007622a9a61 but expected c28d866fdfed5be79f5ea453e04ce887627a8efa ! c28d866..1d793f1 test -> test (unable to update local ref) Also, avoiding updating the whole index means that the performance significantly improves. In my testing the whole time it takes to deploy a branch has been reduced by about 10 seconds.
Update executeCommand() to leave output intact by separating two different concerns: the error and the output itself. Still always set the returnCode to 1 if any error has occurred.
`timeout` is passed now down to executeCommand() but nothing is done with it. Properly make a context and pass it to exec.CommandContext(). Also, while at it add proper debug printing so that output messages *and* the error would appear in the console if `-debug=true` is specified.
Otherwise we might run into an error like this: From https://git.foo.bar/lol.git ! [rejected] dimstoperms -> dimstoperms (non-fast-forward)
xorpaul
reviewed
Apr 16, 2019
| @@ -200,7 +216,6 @@ func executeCommand(command string, timeout int, allowFail bool) ExecResult { | |||
| } | |||
| } else { | |||
| er.returnCode = 1 | |||
| er.output = fmt.Sprint(err) | |||
There was a problem hiding this comment.
Why don't you want to print the error output if there was an error?
There was a problem hiding this comment.
The error and the program's output are two different things. We don't want to overwrite the actual output if it was there and that's why another field, err has been introduced to ExecResult.
|
Cool stuff! I'll verify your changes later when I have more time available. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some set of fixes that I've done to make g10k usable on a large deployment:
git fetch ...instead of doinggit remote update --pruneeach time if we are interested only in one branch. Races are very common when trying to update all of the refs. Errors can still happen however because the webhook uses one goroutine it is very unlikely (imagine an event listener that fires off g10k deployments). Also, if one or more deploys when there >1 concurrent deploys at the same time will fail then it's not the end of the world because the one which "caught" the newest ref will succeed. Still do an initialgit remote update --pruneto download all of the refs and branches.CommandContext()Verification:
Pardon me if my coding style is bad or I did some mistakes.